-
-
Notifications
You must be signed in to change notification settings - Fork 545
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
vk: improve code quality #617
base: master
Are you sure you want to change the base?
vk: improve code quality #617
Conversation
Codecov Report
@@ Coverage Diff @@
## master #617 +/- ##
==========================================
- Coverage 76.92% 76.92% -0.01%
==========================================
Files 317 317
Lines 9630 9641 +11
Branches 962 964 +2
==========================================
+ Hits 7408 7416 +8
- Misses 2071 2073 +2
- Partials 151 152 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
||
def make_sure_access_token_exists(self, method, data): | ||
if 'access_token' in data: | ||
return data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like access_token is always present, maybe the code can be simplified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid it's not, when we call the method from VKAppOAuth2, we don't have access_token.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I missed the isAppUser
call when reviewing, and lack of test coverage there made me think it is not used. Can you add tests for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll add.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blacktrub Any chance to get to this? 😄
Proposed changes
I wanted to improve code quality of a VK back-end, because I had a problem when I wanted to extend it. So I decided it's a good idea to move
vk_api
function to class implementation, so we can use an inheritance with full power.Types of changes
Please check the type of change your PR introduces:
Checklist
Put an
x
in the boxes that apply. You can also fill these out after creatingthe PR. If you're unsure about any of them, don't hesitate to ask. We're here to
help! This is simply a reminder of what we are going to look for before merging
your code.
Other information
Any other information that is important to this PR such as screenshots of how
the component looks before and after the change.